Derive media content types from file content, not the request - #174
Derive media content types from file content, not the request#174snoopdave wants to merge 1 commit into
Conversation
The type an upload declares is treated as a hint, and the stored type is derived from the file name through one shared MediaTypePolicy. Serving applies the other half of the policy: only a short list of passively-rendered formats is sent inline, everything else is sent as an attachment, and every media response carries nosniff. All paths that accept an upload and all paths that serve uploaded media route through the policy, including the entry editor's replacement-body path and the resource servlets' uploaded-media fallback. Files whose type is outside the inline list, such as CSS and JavaScript held as media, now download rather than render. That is a behaviour change and belongs in the release notes. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
Deriving the stored type from the file and refusing to adopt active types is the right model, and MediaTypePolicy is a good single home for it. Two things block it as-is, and a few behaviour changes need disclosing:
- Customized themes break.
WeblogCustomTheme.getResource()never assigns the media file it looks up, so it always returns null (pre-existing bug), andThemeManagerImpl.importThemestores theme resources as media files. Every/resource/css/*.cssandjs/*.jsof a customized theme therefore takes thefromUploadedMediabranch inResourceServlet/PreviewResourceServletand is now served asapplication/octet-stream+Content-Disposition: attachment+nosniff. Browsers refuse stylesheets and scripts with a mismatched type undernosniff, so those blogs render unstyled and without jQuery/Bootstrap after upgrade. (inline) - The AtomPub edit-media GET (
MediaCollection.getMediaResource, served by propono'sAtomServlet) isn't routed through the policy: it setsContent-Typefrom the file name via JAF with nonosniffand no disposition, sopayload.htmluploaded through any of the five policed entry points is still served astext/htmlat/roller-services/app/<handle>/resource/payload.htmlto anyone with cached Basic auth for the realm. Either applyapplyResponseHeadersthere or state that the AtomPub path is out of scope.
Behaviour changes to disclose (the description only mentions CSS/JS): existing image/svg+xml media now downloads instead of rendering (every <img src=".../logo.svg"> in old entries breaks) and new SVG uploads are stored as application/octet-stream so they vanish from the image chooser; image/x-png (legacy IE uploads), image/jpg, image/avif, image/apng and text/plain also become forced downloads; and uploads with no declared type were previously rejected by checkFileType but are now accepted as application/octet-stream.
| String resourceType = this.context.getMimeType( | ||
| resourceRequest.getResourcePath()); | ||
| if (fromUploadedMedia) { | ||
| // Uploaded through the media library, so it is governed by the |
There was a problem hiding this comment.
This branch is also where customized-theme resources land: WeblogCustomTheme.getResource() looks up the media file but never assigns it to resource, so it always returns null, and importTheme stores theme CSS/JS as media files with their original path. resourceType for css/bootstrap.css is text/css, which isn't inline-safe, so the stylesheet goes out as an octet-stream attachment with nosniff and the browser drops it. Fixing WeblogCustomTheme.getResource to return the media file would route these through the theme branch above; alternatively, treat text/css / text/javascript from the servlet-context mime table as theme-authored here as the description already promises.
| * Formats browsers render without executing anything the file carries. | ||
| * SVG is deliberately absent: it is an XML document that can carry script. | ||
| */ | ||
| private static final Set<String> INLINE_TYPES = Collections.unmodifiableSet( |
There was a problem hiding this comment.
Worth listing what this drops compared to response.setContentType(mediaFile.getContentType()) on master. Existing image/svg+xml media (already stored, already embedded in entries with <img>) now downloads instead of rendering, since browsers never sniff SVG; and image/x-png (legacy IE uploads), image/jpg (some XML-RPC clients), image/avif and image/apng are all passive but absent here. If SVG stays out (defensible, it can carry script), the release note should say so explicitly, and I'd add the other image subtypes.
| } | ||
|
|
||
| /** @return true when browsers render this type without executing it */ | ||
| public static boolean isInlineSafe(String contentType) { |
There was a problem hiding this comment.
text/plain with nosniff can't execute anything, so uploaded .txt files (very common as attachments) could stay inline rather than becoming a save-as dialog for an application/octet-stream notes.txt.
| // Replacing the body re-decides the type, on the same | ||
| // terms as the original upload. | ||
| mediaFile.setContentType(MediaTypePolicy.storedTypeFor( | ||
| mediaFile.getName(), this.uploadedFileContentType)); |
There was a problem hiding this comment.
On replace this derives the type from the record's (old) name rather than the uploaded replacement's name (this.uploadedFileName is right there). Replacing photo.jpg with photo.png without renaming stores image/jpeg for PNG bytes and serves them with nosniff; replacing clip.txt with a video keeps text/plain and forces a download. putMedia in MediaCollection has the same mismatch with mf.getName().
| * conclusive, otherwise the declared type if it is not one a | ||
| * browser would act on, otherwise the generic binary type | ||
| */ | ||
| public static String storedTypeFor(String fileName, String declaredType) { |
There was a problem hiding this comment.
Because active declared types collapse to application/octet-stream before canSave / checkFileType run, an admin's content-type rules in uploads.types.forbid (e.g. image/svg+xml,application/xhtml+xml) no longer match those uploads; the file is stored under the generic type instead of being refused. Run the forbid check against the declared type too, or document that forbid rules are now extension-based.
| } | ||
| try { | ||
| return Utilities.getContentTypeFromFileName(fileName); | ||
| } catch (Exception undetermined) { |
There was a problem hiding this comment.
Utilities.getContentTypeFromFileName is backed by javax.activation's default map, which knows about 22 extensions (no pdf, svg, webp, mp4, mp3, zip, css, js). So "the name decides" only holds for those; for everything else the declared type is stored after all (a report.pdf declared application/zip is stored as zip and downloads). The servlets already use the servlet context's mime table (web.xml has a full one); the policy should consult the same table.
| mf.setWeblog(website); | ||
| mf.setName(name); | ||
| mf.setContentType(type); | ||
| mf.setContentType(MediaTypePolicy.storedTypeFor(name, type)); |
There was a problem hiding this comment.
Previously mf.setContentType(null) made checkFileType reject an upload with no type member; storedTypeFor(name, null) now always returns a type, so the upload is accepted as application/octet-stream when the allow list is empty (the default). Probably fine, but it's a loosening the description should mention.
| if (fromUploadedMedia) { | ||
| // Uploaded through the media library, so it is governed by the | ||
| // same policy as any other media response. | ||
| MediaTypePolicy.applyResponseHeaders(response, resourceType, |
There was a problem hiding this comment.
context.getMimeType() returns null for extensions not mapped in web.xml/the container, and applyResponseHeaders turns null into an octet-stream attachment. On master setContentType(null) left the type unset and the browser could still display the file; now any unmapped (or uppercase, on a case-sensitive container) extension downloads in the theme preview.
2 participants
This change derives a stored media file's content type from the file itself
rather than from the type declared with the upload, and serves media inline only
for a small explicit allow-list of passive formats.
What changed
MediaTypePolicyas the single place for the stored type, the inlineallow-list, and the response headers.
opaque names and never adopt an active type (an explicit list plus any
+xmlsuffix).
X-Content-Type-Options: nosniffon every media response.as
application/octet-streamwith an attachment disposition. SVG is excluded.policy.
Note for the release notes: media held as CSS or JavaScript now downloads
instead of loading inline — a user-visible compatibility change.
Tests
MediaTypePolicyTest(9 behavioral cases plus 3 source audits asserting everycaller routes through the policy) covers a file whose declared type does not
match its name, SVG / XHTML and unknown types, genuine images retaining their
type, and
nosniffon every response.